Skip to content

feat(json): add reviver support to GET, MGET, and ARRPOP#3205

Open
JeanSamGirard wants to merge 14 commits into
redis:masterfrom
JeanSamGirard:feat/json-reviver-support
Open

feat(json): add reviver support to GET, MGET, and ARRPOP#3205
JeanSamGirard wants to merge 14 commits into
redis:masterfrom
JeanSamGirard:feat/json-reviver-support

Conversation

@JeanSamGirard
Copy link
Copy Markdown

@JeanSamGirard JeanSamGirard commented Mar 25, 2026

Allows passing an optional JSON.parse reviver function to transformReply. This enables automatic rehydration of types (like Dates) that are not natively supported by JSON.

Modified:

  • JSON.GET: Added reviver to GetOptions
  • JSON.MGET: Added reviver to arguments
  • JSON.ARRPOP: Added reviver to ArrPopOptions
  • generic-transformers: Added reviver to transformRedisJsonReply and transformRedisJsonNullReply arguments

Utilized .preserve to pass reviver through commands.

Description

I need to store arbitrary data structures in redis that may contain date objects or class instances that need to be rehydrated after being stringified, I believe others may have this need.

Commands that return JSON already needed to go through JSON.parse so this looked like a logical and easy feature to implement with the existing code structure. This is why I went ahead with making a pull request to add this feature myself instead of just creating an issue. (No linked issues)

PS: This is my first time contributing so if I'm doing anything wrong please tell me, thank you.

Checklist

  • Does npm test pass with this change (including linting)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?

Note

Low Risk
Backward-compatible optional API on read paths only; behavior unchanged when reviver is omitted.

Overview
Adds optional JSON.parse reviver support when decoding JSON command replies, so callers can rehydrate values (e.g. ISO date strings back to Date) after Redis returns plain JSON text.

Shared helpers transformRedisJsonReply / transformRedisJsonNullReply now take an optional JsonReviver and pass it into JSON.parse. JSON.GET, JSON.MGET, and JSON.ARRPOP expose reviver on options (or as a third argument for mGet); parseCommand stores it on parser.preserve, which the client already forwards into transformReply (including MULTI). ARRPOP options typing is tightened so path/index stay grouped with path. JSDoc and integration tests cover reviver behavior and multi().json.get.

Reviewed by Cursor Bugbot for commit 9b35e7f. Bugbot is set up for automated code reviews on this repo. Configure here.

Allows passing an optional JSON.parse reviver function to transformReply.
This enables automatic rehydration of types (like Dates) that are not natively supported by JSON.

Modified:
- JSON.GET: Added reviver to GetOptions
- JSON.MGET: Added reviver to arguments
- JSON.ARRPOP: Added reviver to ArrPopOptions
- generic-transformers: Added reviver to transformRedisJsonReply and transformRedisJsonNullReply arguments

Utilized .preserve to pass reviver through commands.
@jit-ci
Copy link
Copy Markdown

jit-ci Bot commented Mar 25, 2026

Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset.

In case there are security findings, they will be communicated to you as a comment inside the PR.

Hope you’ll enjoy using Jit.

Questions? Comments? Want to learn more? Get in touch with us.

@nkaradzhov
Copy link
Copy Markdown
Collaborator

Hi @JeanSamGirard, thanks for the PR!

I’d like 2 changes before merge:

  1. JSON.ARRPOP should not require path just to pass reviver.
    Right now, passing { reviver } without path builds invalid
    args.
  2. Please add tests for the new behavior:
  • JSON.GET with reviver
  • JSON.MGET with reviver
  • JSON.ARRPOP with reviver
  • one multi/pipeline case to confirm preserve flows correctly
    there too

After that, this looks good to merge.

P.S. Its generally a good idea to open an issue before any work is done. This way you will make sure the feature is aligned and accepted by the maintainers beforehand, otherwise you are risking investing the time to implement something that could be rejected later.

Comment thread packages/json/lib/commands/GET.spec.ts Outdated
Comment thread packages/json/lib/commands/ARRPOP.ts
Comment thread packages/json/lib/commands/ARRPOP.spec.ts
@JeanSamGirard
Copy link
Copy Markdown
Author

JeanSamGirard commented Mar 26, 2026

Hi @JeanSamGirard, thanks for the PR!

I’d like 2 changes before merge:

  1. JSON.ARRPOP should not require path just to pass reviver.
    Right now, passing { reviver } without path builds invalid
    args.
  2. Please add tests for the new behavior:
  • JSON.GET with reviver
  • JSON.MGET with reviver
  • JSON.ARRPOP with reviver
  • one multi/pipeline case to confirm preserve flows correctly
    there too

After that, this looks good to merge.

P.S. Its generally a good idea to open an issue before any work is done. This way you will make sure the feature is aligned and accepted by the maintainers beforehand, otherwise you are risking investing the time to implement something that could be rejected later.

Hi,

I made the fix for JSON.ARRPOP and added a test using a reviver for the 3 commands. (I'm not sure in which file the test for multi should go)

Unfortunately it seems I'm unable to run them on my local for some reason... I'm on Windows 11 with docker desktop I keep getting "before all" hook for "client.json.get" I think the client isn't able to connect to the container.

PS: Noted in the future I'll make an issue first ;)

PPS: Sorry for all the additional commits, couldn't test locally so I had to trust 😅

Comment thread packages/json/lib/commands/ARRPOP.ts
Comment thread packages/json/lib/commands/GET.spec.ts
@JeanSamGirard
Copy link
Copy Markdown
Author

Hi @nkaradzhov, wasn't sure if I should ping in my previous message.

I ended up adding the multi() test case in packages\json\lib\commands\GET.spec.ts using multi().json.get for the test.

If there's no other changes needed could you please approve so the tests can run?
I would've run the test locally if I could, but I can't get it to work.

Thank you

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Reviewed by Cursor Bugbot for commit d8f9d88. Configure here.

Comment thread packages/json/lib/commands/ARRPOP.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants